Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comboboxのガイドラインを追加 #1152

Merged
merged 20 commits into from
May 22, 2024
Merged

Comboboxのガイドラインを追加 #1152

merged 20 commits into from
May 22, 2024

Conversation

wentzzz
Copy link
Contributor

@wentzzz wentzzz commented May 7, 2024

課題・背景

やったこと

やらなかったこと

  • PageIndexの意匠(リンクであることがわかりづらい)を変更すること

動作確認

Copy link

netlify bot commented May 7, 2024

Deploy Preview for smarthr-design-system ready!

Name Link
🔨 Latest commit eb50181
🔍 Latest deploy log https://app.netlify.com/sites/smarthr-design-system/deploys/664d5f926605a000081b3751
😎 Deploy Preview https://deploy-preview-1152--smarthr-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wentzzz wentzzz marked this pull request as ready for review May 13, 2024 02:51
@wentzzz wentzzz requested review from versionfive, tosiiu and a team as code owners May 13, 2024 02:51
@wentzzz wentzzz requested review from Qs-F and ayumizu May 13, 2024 02:51
@wentzzz wentzzz assigned oti and unassigned oti May 13, 2024
@wentzzz wentzzz requested a review from oti May 13, 2024 02:51
@versionfive
Copy link
Contributor

「SingleComboboxとMultiComboboxのPropsが長大で見辛い」とのことですが、わけたページに中身がなく、Propsだけのページになってしまっているので、これなら長大になっててもまとまってて(ページ分割しなくて)もよいなと思った派です。

Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

上では1つにまとめたほうがいいかも、と書きましたが、
やり方としては内容が重複しても各ページにわけて、ComboBoxページはPageIndexが中心でもよいかもしれません。ComboBoxはコンポーネントではないので。

content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
@ayumizu
Copy link
Contributor

ayumizu commented May 13, 2024

ページ分ける場合、最後のSingleComboBoxとMultiComboBoxへのリンクがタイトルにあることに気づきづらかったです。
タイトルのリンクだけでなく、明示的にリンクっぽい見た目の導線がほしいなと思いました

image

content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
wentzzz and others added 3 commits May 13, 2024 15:52
Co-authored-by: versionfive <64398878+versionfive@users.noreply.github.com>
Co-authored-by: versionfive <64398878+versionfive@users.noreply.github.com>
Co-authored-by: ayumizu <99632029+ayumizu@users.noreply.github.com>
Copy link
Contributor

@tosiiu tosiiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

何点かコメントしました!ページ構成についてはversionfive sanと同意見です!

content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
content/articles/products/components/combo-box/index.mdx Outdated Show resolved Hide resolved
@wentzzz wentzzz requested review from uknmr and tosiiu May 15, 2024 06:36
versionfive
versionfive previously approved these changes May 15, 2024
Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

細かいsuggestionしましたがLGTMです!すごくわかりやすい!

Co-authored-by: versionfive <64398878+versionfive@users.noreply.github.com>
wentzzz and others added 2 commits May 16, 2024 10:47
…x.mdx

Co-authored-by: versionfive <64398878+versionfive@users.noreply.github.com>
….mdx

Co-authored-by: versionfive <64398878+versionfive@users.noreply.github.com>
@wentzzz wentzzz requested a review from versionfive May 16, 2024 01:53
versionfive
versionfive previously approved these changes May 16, 2024
Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tosiiu
tosiiu previously approved these changes May 17, 2024
Copy link
Contributor

@tosiiu tosiiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Qs-F
Qs-F previously approved these changes May 17, 2024
Copy link
Contributor

@Qs-F Qs-F left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

全体的に読みやすかったです!一部コメントしてますがほぼ重箱の隅なので修正判断はお任せします 👍

@wentzzz wentzzz dismissed stale reviews from Qs-F, tosiiu, and versionfive via d679427 May 22, 2024 01:53
tosiiu
tosiiu previously approved these changes May 22, 2024
Copy link
Contributor

@tosiiu tosiiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@Qs-F Qs-F left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@versionfive versionfive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@wentzzz wentzzz dismissed ayumizu’s stale review May 22, 2024 04:07

PageIndexの意匠変更を取り込んで解決したため

@wentzzz wentzzz merged commit a622358 into main May 22, 2024
5 checks passed
@wentzzz wentzzz deleted the add-guideline-combobox branch May 22, 2024 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants